Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

Proposed Soroban-RPC API revisions #289

Closed
wants to merge 6 commits into from
Closed

Proposed Soroban-RPC API revisions #289

wants to merge 6 commits into from

Conversation

paulbellamy
Copy link
Contributor

@paulbellamy paulbellamy commented Jan 27, 2023 •
edited by kalepail
Loading

Moved the proposals from stellar/go#4716, so we have better threading for discussions

Summary

getAccount - stellar/stellar-cli#373

  • Implement in the client.
  • Deprecate & remove from server.

getHealth

  • kept, as the basis for more "system" information on health, like database initialization status

getLedgerEntry - stellar/stellar-rpc#48

  • Replace with getLedgerEntries, so you can fetch multiple in one txn (in the same ledger)

getEvents - stellar/stellar-cli#375

  • keep it as we need server-side filtering for performance.
  • Remove endLedger as a parameter. Just use startLedger and limit
  • Should error if startLedger > latestLedger
  • Should error if startLedger < oldest ledger this node has stored

getTransactionStatus - stellar/stellar-cli#372

  • Add eta field, for more efficient polling (see comment)

sendTransaction - stellar/stellar-cli#372

  • Add eta field, for more efficient polling (see comment)

simulateTransaction - stellar/stellar-cli#363

getLatestLedger - stellar/stellar-cli#376

  • Added closeTime

getNetwork - stellar/stellar-cli#366

  • Add optional friendbotUrl field

requestAirdrop - stellar/stellar-cli#366

  • Remove from soroban-docs. Add friendbotUrl to getNetwork instead

@stellar-jenkins
Copy link

Something went wrong with PR preview build please check

@paulbellamy paulbellamy mentioned this pull request Jan 27, 2023
Closed
@stellar-jenkins
Copy link

Something went wrong with PR preview build please check

tamirms
tamirms reviewed Jan 30, 2023
View reviewed changes
api/methods/getLedgers.mdx Outdated Show resolved Hide resolved
@stellar-jenkins
Copy link

Something went wrong with PR preview build please check

tamirms
tamirms reviewed Jan 30, 2023
View reviewed changes
api/methods/getEvents.mdx Outdated Show resolved Hide resolved
@2opremio
Copy link
Contributor

I wouldn’t remove getHealth(). I was planning on using it to confirm that the database was intialized (otherwise the getLedger and simulareTransaction endpoint won’t be usable).

@2opremio
Copy link
Contributor

Also, regarding getLedgers() I am not sure it makes sense considering we are only going to keep roughly 24hours worth of metadata

@tamirms
Copy link
Contributor

tamirms commented Jan 30, 2023

Also, regarding getLedgers() I am not sure it makes sense considering we are only going to keep roughly 24hours worth of metadata

I think the plan was to expose this method in order to provide ledgers to other horizon / soroban-rpc instances who only want recent / current ledgers

@tsachiherman
Copy link
Contributor

Also, regarding getLedgers() I am not sure it makes sense considering we are only going to keep roughly 24hours worth of metadata

I think the plan was to expose this method in order to provide ledgers to other horizon / soroban-rpc instances who only want recent / current ledgers

The goal is to provide the foundation of moving away from the ledgerclosemeta over file pipe. This isn't committing other components yet, but would allow us to experiment ( in the future ) decoupling the core and horizon so that these won't be required to be hosted on the same machine.

@stellar-jenkins
Copy link

Something went wrong with PR preview build please check

1 similar comment
@stellar-jenkins
Copy link

Something went wrong with PR preview build please check

@paulbellamy
Copy link
Contributor Author

The goal is to provide the foundation of moving away from the ledgerclosemeta over file pipe. This isn't committing other components yet, but would allow us to experiment ( in the future ) decoupling the core and horizon so that these won't be required to be hosted on the same machine.

We could also leave it out for now, wait, and add it when we decide to experiment with this... 🤔

@paulbellamy paulbellamy mentioned this pull request Jan 31, 2023
Closed
@paulbellamy
Copy link
Contributor Author

Conclusion re: getLedgers: Leave it out for now until we know more about the use case, so we don't commit to the wrong API.

@paulbellamy
Copy link
Contributor Author

Going to leave this PR open for now, so we can cherry-pick parts of the doc changes as we make these updates.

@stellar-jenkins
Copy link

Something went wrong with PR preview build please check

@kalepail
Copy link
Contributor

@paulbellamy What's the latest on this PR? Good to close it yet?

@tsachiherman
Copy link
Contributor

@tamirms given that @paulbellamy is out this week, could you help resolving this ticket ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tamirms tamirms tamirms approved these changes

@tsachiherman tsachiherman tsachiherman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@paulbellamy @stellar-jenkins @2opremio @tamirms @tsachiherman @kalepail